Addressing (Post Merge) code review comments for PR 1845 #1883
Conversation
kylasa
requested review from
illsilin,
carlushuang,
qianfengz,
aosewski,
poyenc,
geyyer,
bartekxk,
andriy-ca and
afagaj
as code owners
|
We could create a function "template <typename LayoutA, typename LayoutB, typename LayoutC> |
There was a problem hiding this comment.
@kylasa Please refactor data type selection logic. You could do this in similar way as Thomas pointed. In general IMHO you could achieve this in two ways, where each is equally good. You have to add one another level of function call. Lets say you first differentiate only data type, then you should first call sth like: run_gemm_example_with_dtype with concrete data type used as a tparam and layouts passed as strings args (you'd have to make if...else ladder to select the type) - this is first level. In second level in run_gemm_example_with_dtype function body you'd just create the another if...else ladder but this time you'd differentiate only the selected layout and call final stage function which would proceed with rest of the regular logic.
In general as I said you could do this both ways, either first differentiate data type, then layout, or in reverse order.
Other important thing is @kylasa please make sure that each enabled configuration of data type & layout is producing correct results. If any of them happen to not work, then please comment it with a TODO note.
|
Thanks for your suggestions @ThomasNing and @aosewski. Code changed accordingly. |
|
@kylasa Please also add the test cases for different data types in the test/ck_tile/gemm/test_gemm_pipeline.cpp. |
|
@illsilin As we discussed yesterday, you could try running the benchmark and smoke test for different datatypes on this PR to see if we will put CK TILE GEMM as our CI daily running. The benchmark and smoke test scripts are already there. |
@ThomasNing I believe that in regular CI run we shouldn't run both benchmark & smoke tests - those kind of scripts should be run at much less frequency like daily or weekly.. We have unit-tests for CK Tile GEMM which are already enabled as I asked about that Illia some time ago. |
|
@aosewski Yes, Illia said he will test the running time for the benchmark to decide what's the frequency we should run those tests (in one/two days period). We are just using this PR to add the test script for datatypes except f16. |
Proposed changes
Following PR is used to address post-merge comments for PR 1845 (enabling datatypes for GEMM).
Checklist
Please put an
xinto the boxes that apply. You can also fill these out after creating the PR. If you're not sure, please don't hesitate to ask.clang-formaton all changed filesDiscussion
If this is a relatively large or complex change, feel free to start a discussion by explaining why you chose the solution you did and what alternatives you considered